libstore: Make HTTP binary cache uploads run in constant memory#14390
Merged
Ericson2314 merged 3 commits intomasterfrom Oct 29, 2025
Merged
libstore: Make HTTP binary cache uploads run in constant memory#14390Ericson2314 merged 3 commits intomasterfrom
Ericson2314 merged 3 commits intomasterfrom
Conversation
github-actions
bot
added
store
xokdvium
force-pushed
the
constant-memory-uploads
branch
4 times, most recently
from
312b2c3 to
f569a03
Compare
Ericson2314
reviewed
Oct 28, 2025
Ericson2314
reviewed
Oct 28, 2025
Ericson2314
reviewed
Oct 28, 2025
| if (compressionMethod) { | ||
| data = compress(*compressionMethod, data); | ||
| StringSink sink{}; | ||
| auto compressionSink = makeCompressionSink(*compressionMethod, sink); |
Member
There was a problem hiding this comment.
So future work is makeCompressionSource? :) Or should we use sinkToSource?
Contributor
Author
There was a problem hiding this comment.
sinkToSource don't think we can. Can't use coroutines easily in curl callbacks. I have a feeling that this will be fine for now.
Contributor
Author
There was a problem hiding this comment.
For now I just wanted to avoid copying stuff twice in memory (like logs) and stuffing it straight into a compression sink. That should be slightly more optimal.
Member
There was a problem hiding this comment.
I agree that the boost coroutine stuff here smells like bad luck. Fine to keep this case memory buffered for now.
xokdvium
force-pushed
the
constant-memory-uploads
branch
from
f569a03 to
c0aafe8
Compare
Ericson2314
force-pushed
the
constant-memory-uploads
branch
from
c0aafe8 to
c6c5d96
Compare
Ericson2314
approved these changes
Oct 29, 2025
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This is necessary to make seeking work with libcurl.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Make uploads run in constant memory. Also change the callbacks to be noexcept, since we really don't want to be unwinding the stack in the curl thread. That will definitely corrupt that stack and make nix/curl crash in very bad ways.
Ericson2314
force-pushed
the
constant-memory-uploads
branch
from
c6c5d96 to
cf75079
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
This makes uploads to HTTP binary caches (including the newly reworked S3 store) run in constant memory. Instead of buffering everything in a string we now do uploads from a single-pass (and optionally restartable for seeking) source.
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.